Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PyPIM integration #1091

Merged
merged 5 commits into from
May 4, 2022
Merged

PyPIM integration #1091

merged 5 commits into from
May 4, 2022

Conversation

plule-ansys
Copy link
Contributor

This PR integrates PyPIM in the launch_mapdl function. The intention is that a caller using pymapdl.launch_mapdl() without specific instructions on how to launch it will be redirected to using a remote instance when this is running in a pre-configured environment. This is the workflow targeted in the internal Ansys Lab.

This method will use PyPIM under the condition that:

  • PyPIM is configured: See pypim doc
  • The user did not pass a specific executable path. In that situation, we assume that the caller have specific requirements

In this workflow, most of the configuration (arguments, ...) is ignored. While some of these configuration may be reintroduced later, the initial version of PIM delegates all the control server side.

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #1091 (6ad90e6) into main (27417d0) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 6ad90e6 differs from pull request most recent head 8df3516. Consider uploading reports for the commit 8df3516 to get more accurate results

@@            Coverage Diff             @@
##             main    #1091      +/-   ##
==========================================
+ Coverage   73.02%   73.06%   +0.04%     
==========================================
  Files          43       43              
  Lines        6383     6394      +11     
==========================================
+ Hits         4661     4672      +11     
  Misses       1722     1722              

Copy link
Collaborator

@germa89 germa89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me.

tests/test_launcher_remote.py Show resolved Hide resolved
tests/test_launcher_remote.py Show resolved Hide resolved
@@ -0,0 +1,64 @@
"""Test the PyPIM integration."""
from unittest.mock import create_autospec
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why unittest? I cannot even see when this is installed in pymapdl

Copy link
Collaborator

@akaszynski akaszynski May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unittest is a standard Python library: https://docs.python.org/3/library/unittest.html

That being said, we should be using pytest instead.

This appears to be compatible with pytest, so there's no issue here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fortunately pytest is very unopinionated on how the assertion are done, only on how tests are declared. Here I created the mocks using the built-in unittest (pytest does not have any specific tooling for that), but injected them with the monkeypatch fixture from pytest.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is pytest-mock but yeah... another package.

Copy link
Collaborator

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @plule-ansys!

@germa89 germa89 merged commit 472bae7 into main May 4, 2022
@germa89 germa89 deleted the feat/pim branch May 4, 2022 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants